Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

delay cmj optimizations #475

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

delay cmj optimizations #475

wants to merge 9 commits into from

Conversation

anmonteiro
Copy link
Member

@anmonteiro anmonteiro commented Jan 15, 2023 •
edited
Loading

fixes #464

  • One disadvantage of this change is that there's a slight de-optimization when it comes to computing the necessary require calls (as they're computed before all the J.program optimizations run).
    • the reason for this is that the cmj file needs to store whether the current module is side-effectful, and that depends on other .cmj files too. So we can't just do it when emitting the .js
    • A naive fix for this is to do a 2nd pass to compute the necessary IDs. I'd like to see numbers on the dune integration before we do that, though, cc @rgrinberg @jchavarri

@anmonteiro
Copy link
Member Author

The diff looks a lot better with https://github.com/melange-re/melange/pull/475/files?w=1

@jchavarri
Copy link
Member

A naive fix for this is to do a 2nd pass to compute the necessary IDs. I'd like to see numbers on the dune integration before we do that, though

Even if the benchmarks look good, the main impact on perf for these changes is that more code will be bundled by webpack or other js bundlers unnecessarily, right? So, the trade-off will be between build speed and bundle size?

I am asking to clarify this because I believe that if one uses (module_system es6) from dune, then webpack (or any other bundler) would be able to dead-code eliminate (or "tree-shake") the unused modules. So maybe the addition of extra modules can be mitigated.

jchavarri
jchavarri reviewed Jan 16, 2023
View reviewed changes
Copy link
Member

@jchavarri jchavarri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: the deoptimization:

the reason for this is that the cmj file needs to store whether the current module is side-effectful, and that depends on other .cmj files too. So we can't just do it when emitting the .js

Would it be possible to store at cmj-generation time the other cmjs info required to calculate the "purity" at cmj-time, and use it later at js-generation time?

jscomp/core/lam_compile_main.ml Outdated
(lam : Lambda.lambda) =
(* Actually simplify_lets is kind of global optimization since it requires you
to know whether it's used or not *)
let compile_coercion ~output_prefix (lam : Lambda.lambda) =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "coercion" mean in this context?

jscomp/core/lam_compile_main.ml Outdated Show resolved Hide resolved
@anmonteiro @jchavarri
Update jscomp/core/lam_compile_main.ml
402e7d0 
Co-authored-by: Javier Chávarri <javier.chavarri@gmail.com>
@anmonteiro
Copy link
Member Author

Would it be possible to store at cmj-generation time the other cmjs info required to calculate the "purity" at cmj-time, and use it later at js-generation time?

this is a good idea. I'll play with it this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jchavarri jchavarri jchavarri left review comments

@EduardoRFS EduardoRFS Awaiting requested review from EduardoRFS

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Delay JS program optimizations in Lam_compile_main until JS generation
2 participants
@anmonteiro @jchavarri